Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove bootstrap dependency. #7567

Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jun 28, 2016

Relates to #7364.

Changes

  • Remove Bootstrap NPM dependency.
  • Move Bootstrap files into src/ui/public/styles/bootstrap and update import statements.
  • Remove glyphicons and carousel styles (doesn't look like we're using either).

Next steps

  • I'll make PRs to remove some of the other unused top-level Bootstrap components, e.g. breadcrumbs.

File size

It's worth noting that the original filesize of our compiled CSS was 949kb. Already, by removing the glyphicons and carousel LESS, this filesize has dropped to 890kb.

@tylersmalley
Copy link
Contributor

I spoke with @cjcenizal about bringing bootstrap.less in, while still referencing the less files from node_modules (@import "~bootstrap/less/grid.less";) until we want to modify them. While this would help in the case we want to upgrade, it creates more of a burden for refactoring, which is the ultimate goal. We're not looking to maintain or upgrade alongside bootstrap.

@Bargs
Copy link
Contributor

Bargs commented Jul 1, 2016

@cjcenizal maybe it makes sense to merge this PR and then start removing the other bootstrap components? I'm thinking it's going to take a while to review all of those sub-PRs, and then this one meta-PR is going to have to be re-reviewed with all of them merged. I think it's best to merge things into master incrementally if possible.

@cjcenizal
Copy link
Contributor Author

@Bargs I'm game. @tylersmalley What do you think?

@tylersmalley
Copy link
Contributor

It's probably a good idea. It would be much easier to review the refactoring which was done as individual PRs. This PR as is should not have any impact on the UI as it's just moving the dependency into the repo.

@cjcenizal
Copy link
Contributor Author

@Bargs @tylersmalley OK I updated the description. Can you give me some LGTMs? Then I'll merge.

//
//##

@carousel-text-shadow: 0 1px 2px rgba(0,0,0,.6);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did these go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the carousel classes, there are no more references to these variables so I felt it was safe to remove them.

@Bargs
Copy link
Contributor

Bargs commented Jul 1, 2016

I see some references to glyphicons in ui-bootstrap-tpls.js in the spalger.angular-bootstrap module. Is it really safe to remove these without also removing angular-bootstrap?

Same question applies to carousel now that I search for it.

@Bargs
Copy link
Contributor

Bargs commented Jul 1, 2016

Not really a big deal, but maybe these comments should be updated: https://github.com/elastic/kibana/blob/master/src/ui/public/styles/dark-theme.less#L8

@cjcenizal
Copy link
Contributor Author

@Bargs Great catch re glyphicons. Looks like we may be using two components (daypicker and timepicker) which rely on a few specific glyphicons, so I'll update the PR to retain those. We're not using the other two components (carousel and rating) which use glyphicons.

@cjcenizal
Copy link
Contributor Author

@Bargs I'll put the carousel classes back and remove them along with the Angular stuff in a later PR.

@Bargs
Copy link
Contributor

Bargs commented Jul 1, 2016

Sounds like a plan. I think it'll be nice for historical reasons to have a PR that only moves the files around, without changing anything anyways. That way we have a good base to look at in the commit history.

@cjcenizal cjcenizal force-pushed the refactor/remove-bootstrap-dependency branch from 983cb05 to 0b2a8cc Compare July 1, 2016 22:06
@cjcenizal
Copy link
Contributor Author

@Bargs @tylersmalley Could you please re-review? I put back carousel and glyphicons.

@Bargs
Copy link
Contributor

Bargs commented Jul 1, 2016

LGTM

@Bargs Bargs unassigned Bargs and BigFunger Jul 1, 2016
@cjcenizal cjcenizal force-pushed the refactor/remove-bootstrap-dependency branch from ee44a34 to 00efd32 Compare July 5, 2016 20:31
@tylersmalley
Copy link
Contributor

LGTM

@cjcenizal cjcenizal merged commit 2cf2218 into elastic:master Jul 5, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
…rap-dependency

Remove bootstrap dependency.

Former-commit-id: 2cf2218
@cjcenizal cjcenizal deleted the refactor/remove-bootstrap-dependency branch June 12, 2019 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants